Open
Conversation
…ve method can be used during calculation of data fields using expressions. Added unit tests for the DataFieldValueCalculator
olavsorl
added
feature
Contributor
📝 WalkthroughWalkthroughAdds a data-field calculation feature: new calculator and processor, per-model Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json (1)
26-26: Inconsistent schema URL.This schema URL differs from other test files in the PR. Other files use
https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json, but this one useshttps://altinncdn.no/schemas/json/layout/layout.schema.v1.json.Consider using a consistent schema URL across all test files for maintainability.
Proposed fix
- "$schema": "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json", + "$schema": "https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json` at line 26, Update the inconsistent $schema value in the test JSON by replacing the existing "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json" with the canonical schema URL used elsewhere: "https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json"; locate the "$schema" property (present in the JSON object) and update its string to the canonical toolkit path so all test files use the same schema URL.test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json (1)
9-12: Consider using a calculation-specific schema URL.The
$schemareferencesvalidation.schema.v1.jsonbut this is a calculation configuration. If a dedicated calculation schema exists or is planned, consider using it for clarity and proper validation. This inconsistency appears in multiple test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json` around lines 9 - 12, The test's calculationConfig uses a generic validation.schema.v1.json for "$schema" even though it contains calculation entries (calculations and keys like "form.name"); update the "$schema" value to point to the calculation-specific schema (or a dedicated stub schema for calculations) so tests validate against the correct schema; locate the JSON object named calculationConfig and replace the "$schema" string value currently set to "https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/validation/validation.schema.v1.json" with the proper calculation schema URL (or a local test calculation schema) ensuring all similar test files use the same calculation-specific schema.src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (2)
97-98: Avoid re-fetching form wrapper inside the inner loop.
dataElementis stable in this method, so fetch the wrapper once before iterating resolved fields.♻️ Proposed refactor
- foreach (var (baseField, calculations) in dataFieldCalculations) + var formDataWrapper = await dataAccessor.GetFormDataWrapper(dataElement); + foreach (var (baseField, calculations) in dataFieldCalculations) { @@ - var formDataWrapper = await dataAccessor.GetFormDataWrapper(dataElement); foreach (var calculation in calculations) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 97 - 98, The method in DataFieldValueCalculator repeatedly calls await dataAccessor.GetFormDataWrapper(dataElement) inside the inner loop over resolved fields/calculations; since dataElement is stable, call GetFormDataWrapper(dataElement) once before entering the loop (assign to formDataWrapper) and reuse that variable inside the loop (where calculations and resolved fields are processed), removing the duplicated await calls to avoid unnecessary I/O and improve performance.
14-38: Tighten class surface and remove service-locator dependency.This feature class can likely be
internal sealed, andIDataElementAccessCheckershould be injected directly instead of pulled fromIServiceProvider.♻️ Proposed refactor
-public class DataFieldValueCalculator +internal sealed class DataFieldValueCalculator { @@ - public DataFieldValueCalculator( + public DataFieldValueCalculator( ILogger<DataFieldValueCalculator> logger, ILayoutEvaluatorStateInitializer layoutEvaluatorStateInitializer, IAppResources appResourceService, - IServiceProvider serviceProvider + IDataElementAccessChecker dataElementAccessChecker ) { _logger = logger; _appResourceService = appResourceService; _layoutEvaluatorStateInitializer = layoutEvaluatorStateInitializer; - _dataElementAccessChecker = serviceProvider.GetRequiredService<IDataElementAccessChecker>(); + _dataElementAccessChecker = dataElementAccessChecker; }As per coding guidelines "Use internal accessibility on types by default", "Use sealed for classes unless inheritance is considered a valid use-case", and "register services in DI container properly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 14 - 38, The DataFieldValueCalculator class should be narrowed and avoid the service-locator pattern: change the class declaration to internal sealed (DataFieldValueCalculator) and modify its constructor to accept an IDataElementAccessChecker parameter directly (instead of IServiceProvider), assign it to the _dataElementAccessChecker field, and remove the IServiceProvider parameter and its GetRequiredService call; update any call sites/DI registrations to register and pass IDataElementAccessChecker into the DataFieldValueCalculator constructor.src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs (1)
9-20: Prefer direct constructor injection and a tighter type surface.
IServiceProviderhere introduces service-locator coupling. InjectDataFieldValueCalculatordirectly, and make this typeinternal sealedunless you intentionally expose/extensibility-enable it.♻️ Proposed refactor
-using Microsoft.Extensions.DependencyInjection; namespace Altinn.App.Core.Features.DataProcessing; -public class DataFieldValueCalculatorProcessor : IDataWriteProcessor +internal sealed class DataFieldValueCalculatorProcessor : IDataWriteProcessor { private readonly DataFieldValueCalculator _dataFieldValueCalculator; @@ - public DataFieldValueCalculatorProcessor(IServiceProvider serviceProvider) + public DataFieldValueCalculatorProcessor(DataFieldValueCalculator dataFieldValueCalculator) { - _dataFieldValueCalculator = serviceProvider.GetRequiredService<DataFieldValueCalculator>(); + _dataFieldValueCalculator = dataFieldValueCalculator; }As per coding guidelines "Use internal accessibility on types by default", "Use sealed for classes unless inheritance is considered a valid use-case", and "register services in DI container properly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs` around lines 9 - 20, The class DataFieldValueCalculatorProcessor currently uses IServiceProvider service-locator in its constructor; change it to use direct constructor injection by replacing the IServiceProvider parameter with a DataFieldValueCalculator parameter and assign it to the _dataFieldValueCalculator field inside the constructor, and mark the class as internal sealed (DataFieldValueCalculatorProcessor) to tighten accessibility and prevent inheritance; after this change ensure the DI registration for DataFieldValueCalculatorProcessor is updated to register the concrete type (or the interface it implements) so the container can resolve DataFieldValueCalculator directly.test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
14-15: Use xUnit assertions instead of FluentAssertions in this test project.Please replace
.Should()assertions withAssert.*to align with test standards.♻️ Proposed refactor
-using FluentAssertions; @@ - result.Get(expected.Field).Should().Be(expected.Result.ToObject()); + Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); @@ - result.Get(expected.Field).Should().Be(expected.Result.ToObject()); + Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); @@ - _logger.Collector.GetSnapshot().Select(x => x.Message).Should().Contain(expected.LogMessageWarning); + Assert.Contains(expected.LogMessageWarning, _logger.Collector.GetSnapshot().Select(x => x.Message));As per coding guidelines "
test/**/*.cs: Use xUnit asserts over FluentAssertions in test projects".Also applies to: 73-86, 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 14 - 15, In DataFieldValueCalculatorTests, remove the FluentAssertions dependency (delete the using FluentAssertions) and replace all `.Should()` style assertions in the test class (e.g., occurrences inside DataFieldValueCalculatorTests and the ranges noted around lines 73–86 and 137) with equivalent xUnit Assert calls (e.g., Assert.Equal(expected, actual), Assert.Null(value), Assert.True/False(conditions) or Assert.Throws for exceptions) ensuring each assertion maps to the appropriate Assert.* overload and preserves the original expected vs actual ordering and message semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs`:
- Around line 185-186: Replace the TryAddTransient registration for
IDataWriteProcessor with services.AddTransient<IDataWriteProcessor,
DataFieldValueCalculatorProcessor>() so the core
DataFieldValueCalculatorProcessor is always registered alongside any
app-provided implementations (the code that enumerates
GetAll<IDataWriteProcessor>() in InternalPatchService should therefore see
both). Also mark the DataFieldValueCalculatorProcessor class as sealed (unless
there is an intended inheritance use-case) to follow the coding guideline.
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 40-55: Wrap the Calculate lifecycle in OpenTelemetry
instrumentation: create or use an ActivitySource and a Meter and start a parent
Activity in Calculate (recording taskId and instance identifiers), then for each
loop iteration start a child Activity for the data element (use dataType.Id and
dataElement identifiers) and another nested Activity or span around the call to
CalculateFormData; record attributes/tags like taskId, dataType.Id,
calculationConfig and use Counter/Histogram instruments to emit metrics for
"calculations_started", "calculations_failed", "conversion_failures" and
duration; on any exception from _dataElementAccessChecker.CanRead,
_appResourceService.GetCalculationConfiguration or CalculateFormData catch/log
the exception, set the Activity status to error, increment the failures counter
and rethrow or handle accordingly, and ensure Activities are disposed in finally
blocks so tracing captures end times.
- Around line 81-84: The StartsWith check used in the hiddenFields.Exists
predicate is too broad and can match sibling fields; replace that logic with an
exact-or-descendant check (create a helper like IsSameOrDescendantField) that
returns true if candidate.Equals(hiddenField, StringComparison.Ordinal) or if
candidate.StartsWith(hiddenField, StringComparison.Ordinal) AND candidate.Length
> hiddenField.Length AND the next character is either '.' or '['; update the
predicate that currently uses resolvedField.Field.StartsWith(...) to call this
helper so only exact matches or true descendants are considered.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 124-133: GetResolvedKeys has a nullability/type mismatch with
GetResolvedKeysRecursive: change GetResolvedKeysRecursive's parameter and return
types from string?[] to string[] so callers (including GetResolvedKeys which
passes field.Split('.')) match non-nullable arrays; also replace the invalid
empty-array return syntax (return [];) with a proper empty string array (e.g.
use Array.Empty<string>()) and update any recursive call sites to use the
corrected signature (functions: GetResolvedKeys and GetResolvedKeysRecursive).
In `@src/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cs`:
- Around line 9-26: The stack-allocated buffer rowIndicesSpan (Span<int>
rowIndicesSpan = stackalloc int[200]) can be overflowed when count exceeds 200;
before writing into rowIndicesSpan[count] in the loop inside the method in
ExpressionHelper.cs, add a guard that checks if count >= rowIndicesSpan.Length
and if so throw an InvalidOperationException (or ArgumentOutOfRangeException)
with a clear message like "Too many indices in field: {field}" to fail fast;
ensure this check is performed immediately before assigning
rowIndicesSpan[count] and incrementing count so no out-of-range write occurs.
In `@src/Altinn.App.Core/Models/RawDataFieldValueCalculation.cs`:
- Around line 8-30: Change the two public config model classes to non-public
sealed implementation types: update the declarations of DataFieldCalculation and
RawDataFieldValueCalculation to use internal sealed instead of public so they
are not exposed as extension points; keep their existing members (Condition,
Ref) and nullability as-is and ensure no external code relies on the public
types (adjust any internal usages or tests to the new accessibility).
- Line 13: The non-nullable property Condition on class
RawDataFieldValueCalculation is declared without initialization; mark it as
required or initialize via the class constructor to satisfy
nullable-reference-type rules. Update RawDataFieldValueCalculation by adding the
required modifier to the Condition property (e.g., public required Expression
Condition { get; set; }) or add a constructor that accepts an Expression
parameter and assigns it to Condition so the property cannot remain null at
object creation.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.json`:
- Around line 5-7: The test currently sets the input and expected value for
"form.name" to the same string ("feil"), so the calculator may be skipped and
the test still passes; update the hidden-field.json test case so the input value
for "form.name" is different from the expected "feil" (e.g., set input to
"initial" or empty) to force the calculation to run and produce "feil", and make
the same change for the other identical case referenced (the case at the other
occurrence) so both cases validate actual calculation execution.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 97-98: The method in DataFieldValueCalculator repeatedly calls
await dataAccessor.GetFormDataWrapper(dataElement) inside the inner loop over
resolved fields/calculations; since dataElement is stable, call
GetFormDataWrapper(dataElement) once before entering the loop (assign to
formDataWrapper) and reuse that variable inside the loop (where calculations and
resolved fields are processed), removing the duplicated await calls to avoid
unnecessary I/O and improve performance.
- Around line 14-38: The DataFieldValueCalculator class should be narrowed and
avoid the service-locator pattern: change the class declaration to internal
sealed (DataFieldValueCalculator) and modify its constructor to accept an
IDataElementAccessChecker parameter directly (instead of IServiceProvider),
assign it to the _dataElementAccessChecker field, and remove the
IServiceProvider parameter and its GetRequiredService call; update any call
sites/DI registrations to register and pass IDataElementAccessChecker into the
DataFieldValueCalculator constructor.
In
`@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs`:
- Around line 9-20: The class DataFieldValueCalculatorProcessor currently uses
IServiceProvider service-locator in its constructor; change it to use direct
constructor injection by replacing the IServiceProvider parameter with a
DataFieldValueCalculator parameter and assign it to the
_dataFieldValueCalculator field inside the constructor, and mark the class as
internal sealed (DataFieldValueCalculatorProcessor) to tighten accessibility and
prevent inheritance; after this change ensure the DI registration for
DataFieldValueCalculatorProcessor is updated to register the concrete type (or
the interface it implements) so the container can resolve
DataFieldValueCalculator directly.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json`:
- Around line 9-12: The test's calculationConfig uses a generic
validation.schema.v1.json for "$schema" even though it contains calculation
entries (calculations and keys like "form.name"); update the "$schema" value to
point to the calculation-specific schema (or a dedicated stub schema for
calculations) so tests validate against the correct schema; locate the JSON
object named calculationConfig and replace the "$schema" string value currently
set to
"https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/validation/validation.schema.v1.json"
with the proper calculation schema URL (or a local test calculation schema)
ensuring all similar test files use the same calculation-specific schema.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json`:
- Line 26: Update the inconsistent $schema value in the test JSON by replacing
the existing "https://altinncdn.no/schemas/json/layout/layout.schema.v1.json"
with the canonical schema URL used elsewhere:
"https://altinncdn.no/toolkits/altinn-app-frontend/4/schemas/json/layout/layout.schema.v1.json";
locate the "$schema" property (present in the JSON object) and update its string
to the canonical toolkit path so all test files use the same schema URL.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 14-15: In DataFieldValueCalculatorTests, remove the
FluentAssertions dependency (delete the using FluentAssertions) and replace all
`.Should()` style assertions in the test class (e.g., occurrences inside
DataFieldValueCalculatorTests and the ranges noted around lines 73–86 and 137)
with equivalent xUnit Assert calls (e.g., Assert.Equal(expected, actual),
Assert.Null(value), Assert.True/False(conditions) or Assert.Throws for
exceptions) ensuring each assertion maps to the appropriate Assert.* overload
and preserves the original expected vs actual ordering and message semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4d697be-2a03-453f-922f-325b22590c90
📒 Files selected for processing (23)
src/Altinn.App.Core/Configuration/AppSettings.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/DataLists/InstanceDataListsFactory.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cssrc/Altinn.App.Core/Features/DataProcessing/GenericDataProcessor.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.ApplicationMetadata.Service.cssrc/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Implementation/AppResourcesSI.cssrc/Altinn.App.Core/Internal/App/IAppResources.cssrc/Altinn.App.Core/Internal/Data/IFormDataWrapper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cssrc/Altinn.App.Core/Models/RawDataFieldValueCalculation.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/single-expression-boolean.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/single-expression-number.json
...re.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.json
Show resolved
Hide resolved
19 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (1)
83-87:⚠️ Potential issue | 🟠 MajorUse an exact-or-descendant field check here.
This
StartsWithfilter is too broad: hidingform.namealso suppresses sibling targets likeform.nameResultBoolean. That can silently skip unrelated calculations.🐛 Proposed fix
- if ( - hiddenFields.Exists(d => - d.DataElementIdentifier == resolvedField.DataElementIdentifier - && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) - ) - ) + if ( + hiddenFields.Exists(d => + d.DataElementIdentifier == resolvedField.DataElementIdentifier + && IsSameOrDescendantField(resolvedField.Field, d.Field) + ) + ) { continue; }private static bool IsSameOrDescendantField(string candidate, string hiddenField) { return candidate.Equals(hiddenField, StringComparison.Ordinal) || ( candidate.StartsWith(hiddenField, StringComparison.Ordinal) && candidate.Length > hiddenField.Length && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '[') ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 83 - 87, The current StartsWith check in the hiddenFields.Exists lambda is too broad and hides siblings (e.g., form.name vs form.nameResultBoolean); replace it with an exact-or-descendant check by adding a helper like IsSameOrDescendantField(string candidate, string hiddenField) and use that in the Exists predicate for resolvedField.Field and d.Field; the helper should use StringComparison.Ordinal, return true if candidate.Equals(hiddenField), otherwise ensure candidate.StartsWith(hiddenField, StringComparison.Ordinal) && candidate.Length > hiddenField.Length && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '[') so only true for direct descendants.
🧹 Nitpick comments (1)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
181-186: Add at least one test throughCalculateor the processor.Calling
CalculateFormDatadirectly leaves the public feature path untested:GetCalculationConfiguration,CanRead, per-task data-element iteration, and the top-level activity startup are all bypassed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 181 - 186, The test currently calls CalculateFormData directly which bypasses the public execution path; update or add a test in DataFieldValueCalculatorTests that calls the public Calculate (or the processor entry-point) instead of CalculateFormData so GetCalculationConfiguration, CanRead, per-task data-element iteration and top-level activity startup are exercised; wire up the same test fixture inputs (dataAccessor, dataElement, CalculationConfig) or appropriate mocks for ICalculationProcessor/GetCalculationConfiguration and CanRead, invoke Calculate with the same Task id, and assert the expected side-effects/results to cover the public flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 27-33: The constructor declaration for DataFieldValueCalculator
(the public DataFieldValueCalculator(...) initializer) is misformatted; run
CSharpier (or dotnet format if configured) on the file
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs to
reformat the constructor block so it matches the project's C# formatting rules
and resolves the "Verify dotnet format" failure—ensure the public
DataFieldValueCalculator(...) parameter list and opening brace adhere to
CSharpier's style and then re-run the format/verify step before committing.
- Around line 187-188: ResolveDataFieldCalculation can be made static because it
only uses its parameters and the static field _jsonSerializerOptions; update the
method signature of ResolveDataFieldCalculation to add the static modifier and
verify any callers still compile (no instance state is used), ensuring
references to ResolveDataFieldCalculation and the static _jsonSerializerOptions
remain valid.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 201-208: The recursion should stop when an explicit indexed row is
missing: in the GetResolvedKeysRecursive flow, after calling
GetElementAt(childModelList, groupIndex.Value) and before recursing via
GetResolvedKeysRecursive, check if groupIndex.HasValue and elementAt is null
and, in that case, bail out (return no resolved keys) instead of continuing in
calculation mode; update the branch around the GetElementAt →
GetResolvedKeysRecursive call (and preserve JoinFieldKeyParts usage for valid
elements) so missing indexed rows do not produce keys like "group[99].field".
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 40-51: The test fails to compile because the
DataFieldValueCalculator constructor signature changed to require a telemetry
dependency and the test creates accessCheckerMock but injects a different mock;
update the constructor call to pass a configured telemetry mock (e.g., a
Mock<ITelemetryClient> or whatever telemetry interface your production code
expects) and replace dataElementAccessChecker.Object with
accessCheckerMock.Object so the IDataElementAccessChecker you configure via
accessCheckerMock.Setup(...) is actually injected into DataFieldValueCalculator.
In
`@test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Line 2287: Add backward-compatible forwarding overloads for the original
signatures so existing binaries don't break: implement
DataModelWrapper.GetResolvedKeys(string field) and
LayoutEvaluatorState.GetResolvedKeys(DataReference reference) as simple wrappers
that call the new parameterized overloads (the versions accepting the optional
bool isCalculating) passing isCalculating: false; ensure method names and
parameter types match the original public API exactly so they delegate to the
new methods and preserve binary compatibility.
- Around line 2914-2915: The new abstract method GetCalculationConfiguration on
IAppResources breaks external implementers; to fix, either make it a default
interface method on IAppResources that returns null (add a default
implementation for string? GetCalculationConfiguration(string dataTypeId) =>
null) so existing implementations continue to compile, or extract this member
into a new interface (e.g., IAppResourcesWithCalculation) and implement it only
where needed; update references to call the new interface where calculation
config is required. Ensure you modify the IAppResources declaration (or create
the new interface) and adjust usages accordingly.
---
Duplicate comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 83-87: The current StartsWith check in the hiddenFields.Exists
lambda is too broad and hides siblings (e.g., form.name vs
form.nameResultBoolean); replace it with an exact-or-descendant check by adding
a helper like IsSameOrDescendantField(string candidate, string hiddenField) and
use that in the Exists predicate for resolvedField.Field and d.Field; the helper
should use StringComparison.Ordinal, return true if
candidate.Equals(hiddenField), otherwise ensure
candidate.StartsWith(hiddenField, StringComparison.Ordinal) && candidate.Length
> hiddenField.Length && (candidate[hiddenField.Length] == '.' ||
candidate[hiddenField.Length] == '[') so only true for direct descendants.
---
Nitpick comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 181-186: The test currently calls CalculateFormData directly which
bypasses the public execution path; update or add a test in
DataFieldValueCalculatorTests that calls the public Calculate (or the processor
entry-point) instead of CalculateFormData so GetCalculationConfiguration,
CanRead, per-task data-element iteration and top-level activity startup are
exercised; wire up the same test fixture inputs (dataAccessor, dataElement,
CalculationConfig) or appropriate mocks for
ICalculationProcessor/GetCalculationConfiguration and CanRead, invoke Calculate
with the same Task id, and assert the expected side-effects/results to cover the
public flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1fc19bc-0504-491d-8c67-1c158176a694
📒 Files selected for processing (16)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cssrc/Altinn.App.Core/Internal/Expressions/ExpressionValue.cssrc/Altinn.App.Core/Models/RawDataFieldValueCalculation.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cstest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/single-expression-boolean.jsontest/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/single-expression-number.jsontest/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/component-lookup-hidden.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/shared/component-lookup-hidden.json
- src/Altinn.App.Core/Internal/Expressions/ExpressionHelper.cs
- src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
- src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculatorProcessor.cs
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-field.json
- test/Altinn.App.Core.Tests/Features/DataProcessing/data-field-value-calculator-tests/backend/hidden-page.json
- src/Altinn.App.Core/Internal/Expressions/ExpressionValue.cs
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
Outdated
Show resolved
Hide resolved
.../Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Outdated
Show resolved
Hide resolved
.../Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
40-54:⚠️ Potential issue | 🟠 MajorWrong mock is injected—tests may pass spuriously or fail unexpectedly.
accessCheckerMockis configured to returntrueforCanRead, butdataElementAccessChecker(a separate, unconfigured mock) is injected into the calculator. Since the tests callCalculateFormDatadirectly (bypassingCalculate), the access checker isn't invoked and this bug is currently hidden. If tests are added that callCalculate, they will fail with aMoq.MockException.🐛 Proposed fix
public DataFieldValueCalculatorTests(ITestOutputHelper output) { - var accessCheckerMock = new Mock<IDataElementAccessChecker>(); - accessCheckerMock.Setup(x => x.CanRead(It.IsAny<Instance>(), It.IsAny<DataType>())).ReturnsAsync(true); - - var dataElementAccessChecker = new Mock<IDataElementAccessChecker>(); + var dataElementAccessChecker = new Mock<IDataElementAccessChecker>(); + dataElementAccessChecker + .Setup(x => x.CanRead(It.IsAny<Instance>(), It.IsAny<DataType>())) + .ReturnsAsync(true); var telemetry = new TelemetrySink();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 40 - 54, The tests configure accessCheckerMock to return true for CanRead but accidentally inject an unconfigured dataElementAccessChecker into the DataFieldValueCalculator; replace the injected mock with accessCheckerMock.Object (or remove the unused dataElementAccessChecker) when constructing DataFieldValueCalculator so the configured mock is used; ensure references to CalculateFormData and Calculate remain valid and add/adjust any tests that exercise Calculate to avoid Moq.MockException.src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (1)
188-200:⚠️ Potential issue | 🟠 Major
ResolveDataFieldCalculationshould bestatic(pipeline failure) and has dead code in the string branch.The pipeline is failing because this method doesn't access instance data. Additionally, when
definition.ValueKind == JsonValueKind.String, the parsedstringReferenceis validated but never used—the method proceeds with an emptyRawDataFieldValueCalculationthat will always fail the null-condition check at line 218.🐛 Proposed fix
- private DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) + private static DataFieldCalculation? ResolveDataFieldCalculation(string field, JsonElement definition, ILogger logger) { var rawDataFieldValueCalculation = new RawDataFieldValueCalculation(); if (definition.ValueKind == JsonValueKind.String) { var stringReference = definition.GetString(); if (stringReference == null) { logger.LogError("Could not resolve null reference for calculation for field {Field}", field); return null; } + // TODO: Handle string-based calculation references (e.g., lookup or shorthand syntax) + logger.LogError("String-based calculation definitions are not yet supported for field {Field}", field); + return null; }The
staticmodifier issue was flagged in a previous review. The dead-code concern in the string branch is new.
🧹 Nitpick comments (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (1)
150-153: Redundantloggerparameter shadows instance field.
ParseDataFieldCalculationConfigaccepts anILogger<DataFieldValueCalculator>parameter but the caller always passes_logger(line 73). Since this is an instance method, consider removing the parameter and using_loggerdirectly for consistency.♻️ Proposed simplification
- private Dictionary<string, List<DataFieldCalculation>> ParseDataFieldCalculationConfig( - string rawCalculationConfig, - ILogger<DataFieldValueCalculator> logger - ) + private Dictionary<string, List<DataFieldCalculation>> ParseDataFieldCalculationConfig( + string rawCalculationConfig + ) { using var calculationConfigDocument = JsonDocument.Parse(rawCalculationConfig); var dataFieldCalculations = new Dictionary<string, List<DataFieldCalculation>>();And update the call site at line 73:
- var dataFieldCalculations = ParseDataFieldCalculationConfig(rawCalculationConfig, _logger); + var dataFieldCalculations = ParseDataFieldCalculationConfig(rawCalculationConfig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 150 - 153, The ParseDataFieldCalculationConfig method currently takes an ILogger<DataFieldValueCalculator> parameter that always shadows the instance field _logger; remove the redundant logger parameter from the ParseDataFieldCalculationConfig signature and body, update all call sites (where the method is invoked) to stop passing _logger and rely on the instance field _logger inside the method, and run a quick compile to ensure no remaining references to the removed parameter remain.test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
64-86: Consider consolidating duplicate test methods.
RunDataFieldCalculationTestsForBackendandRunDataFieldCalculationTestsForSharedhave identical implementations—only the test data folder differs. This duplication can be reduced.♻️ Optional: Consolidate into a single parameterized test
+ public static IEnumerable<object[]> GetAllTestFolders() + { + yield return new object[] { "backend" }; + yield return new object[] { "shared" }; + } + [Theory] - [FileNamesInFolderData(["Features", "DataProcessing", "data-field-value-calculator-tests", "backend"])] - public async Task RunDataFieldCalculationTestsForBackend(string fileName, string folder) + [MemberData(nameof(GetAllTestFolders))] + [FileNamesInFolderData(["Features", "DataProcessing", "data-field-value-calculator-tests"])] + public async Task RunDataFieldCalculationTests(string fileName, string folder) { var (result, testCase) = await RunDataFieldCalculatorTest(fileName, folder); foreach (var expected in testCase.Expects) { Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); } } - - [Theory] - [FileNamesInFolderData(["Features", "DataProcessing", "data-field-value-calculator-tests", "shared"])] - public async Task RunDataFieldCalculationTestsForShared(string fileName, string folder) - { - var (result, testCase) = await RunDataFieldCalculatorTest(fileName, folder); - - foreach (var expected in testCase.Expects) - { - Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); - } - }Note: This depends on how
FileNamesInFolderDataworks—if it doesn't support dynamic folder injection, keeping them separate is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 64 - 86, Both test methods RunDataFieldCalculationTestsForBackend and RunDataFieldCalculationTestsForShared are identical except for the folder passed to FileNamesInFolderData; consolidate them into a single parameterized test that takes the folder as a parameter (or uses multiple FileNamesInFolderData attributes) and reuses RunDataFieldCalculatorTest and the same assertion loop, referencing RunDataFieldCalculationTestsForBackend/RunDataFieldCalculationTestsForShared, FileNamesInFolderData, and RunDataFieldCalculatorTest to locate and replace the duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 215-222: The Expected record's properties Field, Result, and
LogMessageWarning are declared as non-nullable but may be omitted during JSON
deserialization, causing null refs; fix by making these properties nullable
(string? Field, ExpressionValue? Result, string? LogMessageWarning) or mark them
required, and then update the test assertion loops (places that access
expected.Result.ToObject() and similar) to null-check expected.Result and
expected.Field/LogMessageWarning before use or handle the null case explicitly
so assertions don't throw.
---
Duplicate comments:
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 40-54: The tests configure accessCheckerMock to return true for
CanRead but accidentally inject an unconfigured dataElementAccessChecker into
the DataFieldValueCalculator; replace the injected mock with
accessCheckerMock.Object (or remove the unused dataElementAccessChecker) when
constructing DataFieldValueCalculator so the configured mock is used; ensure
references to CalculateFormData and Calculate remain valid and add/adjust any
tests that exercise Calculate to avoid Moq.MockException.
---
Nitpick comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 150-153: The ParseDataFieldCalculationConfig method currently
takes an ILogger<DataFieldValueCalculator> parameter that always shadows the
instance field _logger; remove the redundant logger parameter from the
ParseDataFieldCalculationConfig signature and body, update all call sites (where
the method is invoked) to stop passing _logger and rely on the instance field
_logger inside the method, and run a quick compile to ensure no remaining
references to the removed parameter remain.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 64-86: Both test methods RunDataFieldCalculationTestsForBackend
and RunDataFieldCalculationTestsForShared are identical except for the folder
passed to FileNamesInFolderData; consolidate them into a single parameterized
test that takes the folder as a parameter (or uses multiple
FileNamesInFolderData attributes) and reuses RunDataFieldCalculatorTest and the
same assertion loop, referencing
RunDataFieldCalculationTestsForBackend/RunDataFieldCalculationTestsForShared,
FileNamesInFolderData, and RunDataFieldCalculatorTest to locate and replace the
duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f033474-356e-4781-b2bf-4087d5bb89dd
📒 Files selected for processing (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (2)
84-88:⚠️ Potential issue | 🟠 MajorUse exact-or-descendant matching for hidden-field filtering.
Line 87 uses
StartsWith, which can match sibling fields (for exampleform.name2vs hiddenform.name) and skip unrelated calculations.🐛 Proposed fix
- if ( - hiddenFields.Exists(d => - d.DataElementIdentifier == resolvedField.DataElementIdentifier - && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) - ) - ) + if ( + hiddenFields.Exists(d => + d.DataElementIdentifier == resolvedField.DataElementIdentifier + && IsSameOrDescendantField(resolvedField.Field, d.Field) + ) + ) { continue; }+ private static bool IsSameOrDescendantField(string candidate, string hiddenField) + { + if (candidate.Equals(hiddenField, StringComparison.Ordinal)) + { + return true; + } + + return candidate.StartsWith(hiddenField, StringComparison.Ordinal) + && candidate.Length > hiddenField.Length + && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '['); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 84 - 88, In DataFieldValueCalculator, fix the hidden-field filter that currently uses resolvedField.Field.StartsWith(d.Field, ...) so it doesn't wrongly match siblings; change the logic in the hiddenFields.Exists predicate to allow either an exact match (resolvedField.Field == d.Field) or a descendant match (resolvedField.Field starts with d.Field + "." using the same StringComparison), preserving the DataElementIdentifier equality check; update the predicate used where hiddenFields.Exists(...) is called to implement this exact-or-descendant matching.
42-56:⚠️ Potential issue | 🟠 MajorTelemetry coverage is still partial for this feature flow.
Line 44 starts a parent activity, but the new calculation path still lacks per-data-element/per-calculation spans and failure metrics/status tagging. That makes operational troubleshooting much harder for this feature.
As per coding guidelines "
src/**/*.cs: Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 42 - 56, Add per-data-element and per-calculation telemetry around the loop in Calculate: for each iteration create a child span using _telemetry (e.g., StartCalculateActivity or a new StartDataElementCalculationActivity) keyed by dataType.Id and taskId, set attributes for dataType.Id, dataElement.Id and calculationConfig, and record success/failure status and exception details if CalculateFormData throws; also emit a metric or counter for calculation attempts/failures. Instrument CalculateFormData entry/exit with its own span or use the child span to time the call, and ensure the span is ended in a finally block and errors are tagged using OpenTelemetry semantic conventions so per-data-element traces and failure metrics are available for observability.test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs (1)
213-220:⚠️ Potential issue | 🟡 MinorAlign
Expectednullability with actual JSON payloads.
expectsentries can omit properties (see Line 93-95), so non-nullableField,Result, andLogMessageWarningare unsafe and can lead to null dereferences in assertions.🛠️ Proposed fix
public record Expected { - public string Field { get; set; } + public string? Field { get; set; } - public ExpressionValue Result { get; set; } + public ExpressionValue? Result { get; set; } - public string LogMessageWarning { get; set; } + public string? LogMessageWarning { get; set; } }- foreach (var expected in testCase.Expects) - { - Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); - } + foreach (var expected in testCase.Expects) + { + if (expected.Field is not null && expected.Result is not null) + { + Assert.Equal(expected.Result.ToObject(), result.Get(expected.Field)); + } + }As per coding guidelines "
**/*.cs: Use Nullable Reference Types in C# code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs` around lines 213 - 220, The Expected record's properties (Field, Result, LogMessageWarning) are non-nullable but the test JSON can omit them; update the Expected record so each property is nullable (e.g., string? Field, ExpressionValue? Result, string? LogMessageWarning) and adjust any assertions in DataFieldValueCalculatorTests that access these properties to handle nulls (use null-safe checks or explicit assertions) to avoid potential null dereferences when deserializing missing fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 84-88: In DataFieldValueCalculator, fix the hidden-field filter
that currently uses resolvedField.Field.StartsWith(d.Field, ...) so it doesn't
wrongly match siblings; change the logic in the hiddenFields.Exists predicate to
allow either an exact match (resolvedField.Field == d.Field) or a descendant
match (resolvedField.Field starts with d.Field + "." using the same
StringComparison), preserving the DataElementIdentifier equality check; update
the predicate used where hiddenFields.Exists(...) is called to implement this
exact-or-descendant matching.
- Around line 42-56: Add per-data-element and per-calculation telemetry around
the loop in Calculate: for each iteration create a child span using _telemetry
(e.g., StartCalculateActivity or a new StartDataElementCalculationActivity)
keyed by dataType.Id and taskId, set attributes for dataType.Id, dataElement.Id
and calculationConfig, and record success/failure status and exception details
if CalculateFormData throws; also emit a metric or counter for calculation
attempts/failures. Instrument CalculateFormData entry/exit with its own span or
use the child span to time the call, and ensure the span is ended in a finally
block and errors are tagged using OpenTelemetry semantic conventions so
per-data-element traces and failure metrics are available for observability.
In
`@test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs`:
- Around line 213-220: The Expected record's properties (Field, Result,
LogMessageWarning) are non-nullable but the test JSON can omit them; update the
Expected record so each property is nullable (e.g., string? Field,
ExpressionValue? Result, string? LogMessageWarning) and adjust any assertions in
DataFieldValueCalculatorTests that access these properties to handle nulls (use
null-safe checks or explicit assertions) to avoid potential null dereferences
when deserializing missing fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86bc4e8c-f0d4-4648-96a6-a3606589c61d
📒 Files selected for processing (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 150-153: Change the ParseDataFieldCalculationConfig method to a
static method since it only uses parameters and calls the static
ResolveDataFieldCalculation; update its signature to "private static
Dictionary<string, List<DataFieldCalculation>>
ParseDataFieldCalculationConfig(...)" and ensure any callers invoke it as a
static method (or via the class name) instead of relying on an instance; this
removes the CA1822 warning while keeping the call to ResolveDataFieldCalculation
intact.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 113-122: Add XML documentation for the public method
GetResolvedKeys(string field) to satisfy CS1591: provide a <summary> describing
that it returns resolved data model keys for the given dotted field path, a
<param name="field"> describing the expected dotted field string, and a
<returns> describing the returned string[]; match wording and style used by the
existing overload of GetResolvedKeys to keep consistency with other XML docs in
DataModelWrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e10ec47f-4ccd-49b9-a1aa-b70d738855e4
📒 Files selected for processing (3)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs (2)
85-88:⚠️ Potential issue | 🟠 MajorUse exact-or-descendant path matching for hidden fields.
StartsWithon Line 87 is too broad and can hide sibling fields unintentionally. Match only exact field or true descendants (./[boundary), and useStringComparison.Ordinal.🐛 Proposed fix
- if ( - hiddenFields.Exists(d => - d.DataElementIdentifier == resolvedField.DataElementIdentifier - && resolvedField.Field.StartsWith(d.Field, StringComparison.InvariantCulture) - ) - ) + if ( + hiddenFields.Exists(d => + d.DataElementIdentifier == resolvedField.DataElementIdentifier + && IsSameOrDescendantField(resolvedField.Field, d.Field) + ) + ) { continue; }+ private static bool IsSameOrDescendantField(string candidate, string hiddenField) + { + if (candidate.Equals(hiddenField, StringComparison.Ordinal)) + { + return true; + } + + return candidate.StartsWith(hiddenField, StringComparison.Ordinal) + && candidate.Length > hiddenField.Length + && (candidate[hiddenField.Length] == '.' || candidate[hiddenField.Length] == '['); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 85 - 88, The hidden-field check in DataFieldValueCalculator (the lambda passed to hiddenFields.Exists comparing d.DataElementIdentifier and resolvedField.Field) currently uses StartsWith and should be tightened: replace the StartsWith(resolvedField.Field, StringComparison.InvariantCulture) logic with an exact match OR a descendant match that requires the next character after the prefix to be '.' or '[' and use StringComparison.Ordinal for comparisons; in other words, return true if d.Field == resolvedField.Field (ordinal) or if d.Field.Length > resolvedField.Field.Length && d.Field.StartsWith(resolvedField.Field, StringComparison.Ordinal) && (d.Field[resolvedField.Field.Length] == '.' || d.Field[resolvedField.Field.Length] == '[').
42-57:⚠️ Potential issue | 🟠 MajorTelemetry coverage is still too thin for this feature flow.
Only a top-level activity is started (Line 44). Per-data-element spans and failure metrics around access checks/config load/calculation execution are still missing.
As per coding guidelines "
src/**/*.cs: Comprehensive telemetry instrumentation should be included in feature implementations using OpenTelemetry".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs` around lines 42 - 57, Add fine-grained telemetry inside Calculate: for each data element (loop over dataType/dataElement) start a per-data-element activity/span (use _telemetry.StartCalculateActivity or a new StartCalculateElementActivity) with attributes dataType.Id and taskId, then wrap the access check (_dataElementAccessChecker.CanRead), config load (_appResourceService.GetCalculationConfiguration), and the call to CalculateFormData in short child spans or timed metrics; on access denial, config missing, or CalculateFormData exceptions record failures via telemetry.RecordException or increment failure counters and set span status accordingly, and ensure the per-element activity is always ended in a finally block so metrics and statuses are emitted.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs (1)
124-155: Consider consolidating the twoGetResolvedKeysoverloads.Both overloads duplicate null-check and split logic. Let
GetResolvedKeys(string field)delegate toGetResolvedKeys(field, isCalculating: false)to keep one path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs` around lines 124 - 155, Consolidate the duplicated logic by making the single-parameter GetResolvedKeys(string field) delegate to the two-parameter overload: have GetResolvedKeys(field) simply return GetResolvedKeys(field, isCalculating: false) (preserving the empty-array return when _dataModel is null inside the two-parameter overload), and remove the duplicated null-check and Split('.') logic from the single-parameter method so all logic flows through GetResolvedKeys(string field, bool isCalculating) which calls GetResolvedKeysRecursive(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 189-197: The code in DataFieldValueCalculator reads a string JSON
definition into stringReference but never assigns it to
rawDataFieldValueCalculation.Condition, causing valid string-based calculations
to be treated as missing later; update the branch that checks
definition.ValueKind == JsonValueKind.String to set
rawDataFieldValueCalculation.Condition = stringReference (and keep the null
check/logging), and ensure the later validation that currently rejects missing
conditions (the check around rawDataFieldValueCalculation.Condition) accepts the
assigned string so string-form entries from calculation.json are executed.
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 201-213: The current logic in GetResolvedKeysRecursive
(DataModelWrapper) sets elementType =
childType.GetGenericArguments().FirstOrDefault() ?? typeof(object), which fails
for arrays or non-generic enumerables; update the inference to handle arrays and
runtime instances: first check childType.IsArray and use
childType.GetElementType(), then check for non-generic IEnumerable and if still
null fall back to using child?.GetType() (the actual runtime type of the child
from childModelList) to derive the element type before recursing so the
recursive property lookup uses the real element type instead of object.
---
Duplicate comments:
In `@src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs`:
- Around line 85-88: The hidden-field check in DataFieldValueCalculator (the
lambda passed to hiddenFields.Exists comparing d.DataElementIdentifier and
resolvedField.Field) currently uses StartsWith and should be tightened: replace
the StartsWith(resolvedField.Field, StringComparison.InvariantCulture) logic
with an exact match OR a descendant match that requires the next character after
the prefix to be '.' or '[' and use StringComparison.Ordinal for comparisons; in
other words, return true if d.Field == resolvedField.Field (ordinal) or if
d.Field.Length > resolvedField.Field.Length &&
d.Field.StartsWith(resolvedField.Field, StringComparison.Ordinal) &&
(d.Field[resolvedField.Field.Length] == '.' ||
d.Field[resolvedField.Field.Length] == '[').
- Around line 42-57: Add fine-grained telemetry inside Calculate: for each data
element (loop over dataType/dataElement) start a per-data-element activity/span
(use _telemetry.StartCalculateActivity or a new StartCalculateElementActivity)
with attributes dataType.Id and taskId, then wrap the access check
(_dataElementAccessChecker.CanRead), config load
(_appResourceService.GetCalculationConfiguration), and the call to
CalculateFormData in short child spans or timed metrics; on access denial,
config missing, or CalculateFormData exceptions record failures via
telemetry.RecordException or increment failure counters and set span status
accordingly, and ensure the per-element activity is always ended in a finally
block so metrics and statuses are emitted.
---
Nitpick comments:
In `@src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs`:
- Around line 124-155: Consolidate the duplicated logic by making the
single-parameter GetResolvedKeys(string field) delegate to the two-parameter
overload: have GetResolvedKeys(field) simply return GetResolvedKeys(field,
isCalculating: false) (preserving the empty-array return when _dataModel is null
inside the two-parameter overload), and remove the duplicated null-check and
Split('.') logic from the single-parameter method so all logic flows through
GetResolvedKeys(string field, bool isCalculating) which calls
GetResolvedKeysRecursive(...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e33e47fa-3bee-44da-89cb-85f9e4c4f8c3
📒 Files selected for processing (5)
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cssrc/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cssrc/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cstest/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Core.Tests/Features/DataProcessing/DataFieldValueCalculatorTests.cs
src/Altinn.App.Core/Features/DataProcessing/DataFieldValueCalculator.cs
Outdated
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added logic for setting data field by expression through an IDataWriteProcessor implementation called DataFieldValueCalculator. This data processor consumes calculation.json files where expressions that evaluates and sets data model fields is defined. Much of the logic is just a copy of whats found in ExpressionValidation.
Related PRs:
calculation.schema.V1.json - Altinn/app-frontend-react#4048
Docs - Altinn/altinn-studio-docs#2763
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Telemetry